Persist IP addresses related to VM access via CPVM #9534

bernardodemarco · 2024-08-15T14:35:51Z

Description

This PR proposes to persist IP addresses related to VM access via CPVM. Specifically, it stores the IP address of the client accessing a VM and the IP address of the console endpoint creator. To achieve this, the cloud.console_session table was extended with two new columns: client_address, which stores the client's IP address, and console_endpoint_creator_address, which captures the IP address of the console endpoint creator.

These IP addresses were already being captured for logging and validation purposes. The console endpoint creator's IP is captured here, at the start of the CreateConsoleEndpointCmd execution:

cloudstack/api/src/main/java/org/apache/cloudstack/api/command/user/consoleproxy/CreateConsoleEndpointCmd.java

Lines 65 to 68 in 47a6b70

    
           @Override 
        
           public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException { 
        
               String clientAddress = getClientAddress(); 
        
               ConsoleEndpoint endpoint = consoleManager.generateConsoleEndpoint(vmId, extraSecurityToken, clientAddress);

The client address is captured by calling session.getRemoteAddress().getAddress().getHostAddress() in this method, that gets executed when the client connects to the console:

cloudstack/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java

Lines 157 to 160 in 47a6b70

    
           private boolean checkSessionSourceIp(final Session session, final String sourceIP) throws IOException { 
        
               // Verify source IP 
        
               String sessionSourceIP = session.getRemoteAddress().getAddress().getHostAddress(); 
        
               logger.info("Get websocket connection request from remote IP : " + sessionSourceIP);

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Screenshots (if appropriate):

How Has This Been Tested?

I manually added the client_address and console_endpoint_creator_address columns to the cloud.console_session table.
I generated a VM console endpoint.
I executed the following query in order to validate whether the stored IP was correct.

SELECT * FROM `cloud`.`console_session` ORDER BY created DESC LIMIT 1;

id	uuid	created	account_id	user_id	instance_id	host_id	acquired	removed	client_address	console_endpoint_creator_address
2	77f08cea-8068-471f-9464-44810e1ef545	2024-08-06 17:57:05	2	2	14	1	NULL	NULL	NULL	192.168.122.1

As can be noticed, the console_endpoint_creator_address column was populated accordingly, whereas the client_adress was still empty since the VM had not been accessed yet.

I accessed the VM via CPVM.
I executed the following query in order to validate whether the client stored IP was correct.

SELECT * FROM `cloud`.`console_session` ORDER BY created DESC LIMIT 1;

id	uuid	created	account_id	user_id	instance_id	host_id	acquired	removed	client_address	console_endpoint_creator_address
2	77f08cea-8068-471f-9464-44810e1ef545	2024-08-06 17:57:05	2	2	14	1	NULL	NULL	192.168.122.1	192.168.122.1

As can be noticed, the IP address of the client that accessed the VM through CPVM was persisted correctly.

bernardodemarco · 2024-08-15T14:41:32Z

@blueorangutan package

blueorangutan · 2024-08-15T14:42:03Z

@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

codecov · 2024-08-15T14:47:58Z

Codecov Report

Attention: Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.

Project coverage is 15.77%. Comparing base (501d8c1) to head (1ed976f).

Files with missing lines	Patch %	Lines
...a/src/main/java/com/cloud/vm/ConsoleSessionVO.java	0.00%	12 Missing ⚠️
.../agent/api/ConsoleAccessAuthenticationCommand.java	0.00%	8 Missing ⚠️
...udstack/consoleproxy/ConsoleAccessManagerImpl.java	0.00%	5 Missing ⚠️
...nt/resource/consoleproxy/ConsoleProxyResource.java	0.00%	2 Missing ⚠️
...n/java/com/cloud/vm/dao/ConsoleSessionDaoImpl.java	0.00%	2 Missing ⚠️
...ain/java/com/cloud/consoleproxy/AgentHookBase.java	0.00%	2 Missing ⚠️
...main/java/com/cloud/consoleproxy/ConsoleProxy.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9534      +/-   ##
============================================
- Coverage     15.77%   15.77%   -0.01%     
  Complexity    12538    12538              
============================================
  Files          5621     5621              
  Lines        491556   491578      +22     
  Branches      60227    62625    +2398     
============================================
- Hits          77562    77561       -1     
- Misses       405537   405559      +22     
- Partials       8457     8458       +1

Flag	Coverage Δ
uitests	`4.05% <ø> (ø)`
unittests	`16.59% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

blueorangutan · 2024-08-15T15:53:50Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10674

github-actions · 2024-08-27T22:15:57Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland · 2024-09-02T08:25:46Z

@bernardodemarco , I understand what and how, but have a hard time to get the why of this PR. Can you expand on that please?

bernardodemarco · 2024-09-02T18:09:51Z

@bernardodemarco , I understand what and how, but have a hard time to get the why of this PR. Can you expand on that please?

The main idea behind the proposed enhancement is to help operators better identify who accessed a virtual machine console.

The cloud.console_session table already stores data relating to who (account_id and user_id) accessed a given virtual machine (instance_id), as well as when that happened (created and removed). By adding the client_address and console_endpoint_creator_address fields, it'll be easier to identify who accessed the console, which can be extremely helpful for security and auditing purposes.

DaanHoogland · 2024-09-02T19:17:32Z

@bernardodemarco , I understand what and how, but have a hard time to get the why of this PR. Can you expand on that please?

The main idea behind the proposed enhancement is to help operators better identify who accessed a virtual machine console.

The cloud.console_session table already stores data relating to who (account_id and user_id) accessed a given virtual machine (instance_id), as well as when that happened (created and removed). By adding the client_address and console_endpoint_creator_address fields, it'll be easier to identify who accessed the console, which can be extremely helpful for security and auditing purposes.

thanks, makes sense.

DaanHoogland · 2024-09-02T19:21:19Z

@nvazquez can you have a look at this one?

github-actions · 2024-09-05T04:05:11Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

nvazquez

Code LGTM

sureshanaparti

clgtm

DaanHoogland · 2024-09-09T11:50:13Z

@blueorangutan package

blueorangutan · 2024-09-09T11:52:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-09-09T13:00:43Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11055

github-actions · 2024-09-09T13:31:11Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland · 2024-09-10T08:46:21Z

@blueorangutan package

blueorangutan · 2024-09-10T08:48:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

weizhouapache · 2024-09-10T09:33:03Z

client_address is always NULL in my testing.
can you check ? @bernardodemarco cc @DaanHoogland

blueorangutan · 2024-09-10T09:57:21Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11071

DaanHoogland · 2024-09-10T12:50:31Z

@blueorangutan test keepEnv

blueorangutan · 2024-09-10T12:52:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

bernardodemarco · 2024-09-10T19:10:33Z

client_address is always NULL in my testing.

I tested the PR again, following the steps provided on the description, and the IP addresses were stored correctly.

It's important to remember that if there were any running CPVMs prior to the environment update, then they would need to be recreated in order to test if the client_address column is being populated correctly.

Furthermore, it's interesting taking a look at the CPVM logs. The following logs were generated during my tests, which indicates that the client IP 192.168.122.1 was captured correctly.

2024-09-10T17:19:38,330 INFO  [cloud.consoleproxy.ConsoleProxyNoVNCHandler] (qtp2090537387-40:[]) Verifying session
source IP 192.168.122.1 from WebSocket connection request.

2024-09-10T17:19:38,331 DEBUG [cloud.consoleproxy.ConsoleProxyNoVNCHandler] (qtp2090537387-40:[]) Session source IP
192.168.122.1 has been verified successfully.

@weizhouapache, if the problem continues happening, could you please describe your tests steps and share the CPVM logs?

weizhouapache · 2024-09-10T21:57:33Z

client_address is always NULL in my testing.

I tested the PR again, following the steps provided on the description, and the IP addresses were stored correctly.

It's important to remember that if there were any running CPVMs prior to the environment update, then they would need to be recreated in order to test if the client_address column is being populated correctly.

Furthermore, it's interesting taking a look at the CPVM logs. The following logs were generated during my tests, which indicates that the client IP 192.168.122.1 was captured correctly.
2024-09-10T17:19:38,330 INFO  [cloud.consoleproxy.ConsoleProxyNoVNCHandler] (qtp2090537387-40:[]) Verifying session
source IP 192.168.122.1 from WebSocket connection request.

2024-09-10T17:19:38,331 DEBUG [cloud.consoleproxy.ConsoleProxyNoVNCHandler] (qtp2090537387-40:[]) Session source IP
192.168.122.1 has been verified successfully.
@weizhouapache, if the problem continues happening, could you please describe your tests steps and share the CPVM logs?

Thanks @bernardodemarco
It works after patching the cpvm

blueorangutan · 2024-09-11T03:38:34Z

[SF] Trillian test result (tid-11441)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 50973 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9534-t11441-kvm-ol8.zip
Smoke tests completed. 139 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_migrate_VM_and_root_volume	`Error`	97.32	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	50.60	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	0.08	test_vm_life_cycle.py
test_01_migrate_vm_strict_tags_success	`Error`	0.23	test_vm_strict_host_tags.py
test_02_migrate_vm_strict_tags_failure	`Error`	0.23	test_vm_strict_host_tags.py

DaanHoogland · 2024-09-11T07:55:31Z

@JoaoJandre , it looks like this is ready. should we merge it before 4.20?

JoaoJandre · 2024-09-11T12:59:09Z

@JoaoJandre , it looks like this is ready. should we merge it before 4.20?

I think we should leave this for 4.21. We're only accepting bugfixes currently.

boring-cyborg bot added component:agent component:api component:console-proxy labels Aug 15, 2024

bernardodemarco requested review from nvazquez and sureshanaparti August 16, 2024 13:36

github-actions bot added the status:has-conflicts label Aug 27, 2024

bernardodemarco force-pushed the persist-client-ip-on-cpvm-connection branch from 4d62ae2 to cdec550 Compare September 1, 2024 14:41

github-actions bot removed the status:has-conflicts label Sep 1, 2024

DaanHoogland added the status:needs-testing label Sep 2, 2024

github-actions bot added the status:has-conflicts label Sep 5, 2024

bernardodemarco force-pushed the persist-client-ip-on-cpvm-connection branch from cdec550 to e49980f Compare September 7, 2024 15:36

github-actions bot removed the status:has-conflicts label Sep 7, 2024

DaanHoogland added this to the 4.20.0.0 milestone Sep 9, 2024

nvazquez approved these changes Sep 9, 2024

View reviewed changes

sureshanaparti approved these changes Sep 9, 2024

View reviewed changes

github-actions bot added the status:has-conflicts label Sep 9, 2024

bernardodemarco added 3 commits September 9, 2024 16:41

persist IP address that has accessed the CPVM console

8c0bb2b

persist console endpoint creator IP address

cf050aa

add SQL DDL queries

1ed976f

bernardodemarco force-pushed the persist-client-ip-on-cpvm-connection branch from e49980f to 1ed976f Compare September 9, 2024 19:41

github-actions bot removed the status:has-conflicts label Sep 9, 2024

JoaoJandre modified the milestones: 4.20.0.0, 4.21.0.0 Sep 10, 2024

weizhouapache approved these changes Sep 10, 2024

View reviewed changes

DaanHoogland removed the status:needs-testing label Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist IP addresses related to VM access via CPVM #9534

Persist IP addresses related to VM access via CPVM #9534

bernardodemarco commented Aug 15, 2024

bernardodemarco commented Aug 15, 2024

blueorangutan commented Aug 15, 2024

codecov bot commented Aug 15, 2024 •

edited

Loading

blueorangutan commented Aug 15, 2024

github-actions bot commented Aug 27, 2024

DaanHoogland commented Sep 2, 2024

bernardodemarco commented Sep 2, 2024

DaanHoogland commented Sep 2, 2024

DaanHoogland commented Sep 2, 2024

github-actions bot commented Sep 5, 2024

nvazquez left a comment

sureshanaparti left a comment

DaanHoogland commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

github-actions bot commented Sep 9, 2024

DaanHoogland commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

weizhouapache commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

DaanHoogland commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

bernardodemarco commented Sep 10, 2024

weizhouapache commented Sep 10, 2024

blueorangutan commented Sep 11, 2024

DaanHoogland commented Sep 11, 2024

JoaoJandre commented Sep 11, 2024

	@Override
	public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException {
	String clientAddress = getClientAddress();
	ConsoleEndpoint endpoint = consoleManager.generateConsoleEndpoint(vmId, extraSecurityToken, clientAddress);

	private boolean checkSessionSourceIp(final Session session, final String sourceIP) throws IOException {
	// Verify source IP
	String sessionSourceIP = session.getRemoteAddress().getAddress().getHostAddress();
	logger.info("Get websocket connection request from remote IP : " + sessionSourceIP);

Persist IP addresses related to VM access via CPVM #9534

Are you sure you want to change the base?

Persist IP addresses related to VM access via CPVM #9534

Conversation

bernardodemarco commented Aug 15, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Screenshots (if appropriate):

How Has This Been Tested?

bernardodemarco commented Aug 15, 2024

blueorangutan commented Aug 15, 2024

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

blueorangutan commented Aug 15, 2024

github-actions bot commented Aug 27, 2024

DaanHoogland commented Sep 2, 2024

bernardodemarco commented Sep 2, 2024

DaanHoogland commented Sep 2, 2024

DaanHoogland commented Sep 2, 2024

github-actions bot commented Sep 5, 2024

nvazquez left a comment

Choose a reason for hiding this comment

sureshanaparti left a comment

Choose a reason for hiding this comment

DaanHoogland commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

github-actions bot commented Sep 9, 2024

DaanHoogland commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

weizhouapache commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

DaanHoogland commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

bernardodemarco commented Sep 10, 2024

weizhouapache commented Sep 10, 2024

blueorangutan commented Sep 11, 2024

DaanHoogland commented Sep 11, 2024

JoaoJandre commented Sep 11, 2024

codecov bot commented Aug 15, 2024 •

edited

Loading